Implement lossless From methods for Duration conversions #342
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #342 +/- ##
==========================================
+ Coverage 84.01% 84.10% +0.09%
==========================================
Files 22 23 +1
Lines 3691 3675 -16
==========================================
- Hits 3101 3091 -10
+ Misses 590 584 -6 ☔ View full report in Codecov by Sentry. |
| Duration::compose(sign, days, hours, minutes, seconds, milli, us, 0).to_seconds(); | ||
| std::time::Duration::new(above_ns_f64 as u64, nano as u32) | ||
| } | ||
| const NANOS_PER_SEC: u128 = std::time::Duration::from_secs(1).as_nanos(); |
There was a problem hiding this comment.
I think a simpler way to implement this functionality is by using the to_unit function on the hf_duration.
I haven't tested the following, so consider this a draft.
let seconds = hf_duration.to_seconds().floor() as u64; // Somehow check that this won't fail
let subsec_nanos = (hf_duration - Unit::Second * seconds).to_unit(Unit::Nanoseconds).floor() as u32; // Also check that this won't fail, probably using try_from as you initially recommended
std::time::Duration(seconds, subsec_nanos)There was a problem hiding this comment.
There's a better way to handle the subsec_nanos than what I wrote above. After the subtraction, we know that the duration has zero centuries. So, we can safely ignore that part and operate directly on nanoseconds.
use crate::NANOSECONDS_PER_SECOND; // https://docs.rs/hifitime/latest/hifitime/constant.NANOSECONDS_PER_SECOND.html
let subsec_nanos: u64 = (hf_duration - Unit::Second * seconds).nanoseconds / NANOSECONDS_PER_SECOND;
// Finally convert this to u32 safely.
|
Thanks for your PR @Wollaston ! I've proposed a simpler way to have a lossless conversion to std::time::Duration: it's lossless because the operations on Is there a way we could test this? |
|
Thanks for the review @ChristopherRabotin! In the original issue #337, part of the concern was the conversion from ints to floats to ints, which is lossy with the float conversion. In your draft, As to the updated I agree it makes sense to use the As to tests, I can draft some unit tests in the module (it only has these two implementations) and/or add some doc tests to some updated documentation for these implementations. I am not sure if there is a pattern you prefer - just let me know on that. They could test that the updated implementations are in fact lossless as a start. I can update the PR accordingly once those questions are settled, and then squash the commits. Many thanks! |
|
You're right. Let's go ahead with your original implementation using the crate's NANOSECONDS_PER_SECOND constant. As for negative durations, where we'd want to return |
|
Sounds good! Just to clarify, we still want to return And for the tests, do you have a preference in how the tests are added, or just that they are? Thanks! |
|
Yes, I think it makes sense to return By the way, no need to squash the commits, I tend to just merge all of the commits. |
This works towards closing #337 by making the implementations of
From<std::time::Duration> for DurationandFrom<Duration> for std::time::Durationlossless. Thank you!